Update play mvc to use AdviceScope #13140

jaydeluca · 2025-01-31T11:19:25Z

~~Related to #7345~~

Related to #13031

Pivoting from doing a code attribute addition to instead just migrating to using AdviceScope since the resulting code attributes were not useful. If we can come up with some other solution for the code attributes I can do a followup.

trask · 2025-01-31T16:27:19Z

Is this the right approach?

👍

cc @SylvainJuge

laurit · 2025-02-01T12:31:00Z

...rc/test/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/server/PlayServerTest.java

+            equalTo(CODE_NAMESPACE, "play.api.mvc.ActionBuilder$$anon$2"),
+            equalTo(CODE_FUNCTION, "apply"));


In my opinion this is not useful. It points to framework code and is the same for all endpoints.

I tend to agree with Lauri here, if the value is the same for all spans then it does not provide much value for the end-user as with it they can't guess which part of the application code is related to the span.

We will very likely have exactly the same kind of issues with Webflux or other reactive frameworks where the application code is provided through nested lambdas and can't be described with a dedicated class/method. For those cases, the "span identity" should be provided through other attributes for example HTTP verb, path or span name which allows the end-user to understand the link with application code.

If we capture this technical and implementation-specific information, do we have a way to make any good use of it ?

One example I could think of is if we attempt to correlate those attributes with another source of information like inferred spans or profiling, however for those the correlation would be implemented through trace and span ID rather than on those code-level attributes.

I think this example is really interesting from the perspective of semconv attributes clarification as there are at least two distinct use-cases for those attributes:

provide more information about a span for the end-user: we may recommend to capture information that is related to the application code rather than framework/library details

capture "technical information" in a structured way that could not be described otherwise: profiling, inferred spans, ...

...avaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/ActionScope.java

…mentation into add-code-attributes-play-mvc

…semconv

...rc/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/ActionInstrumentation.java

...avaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/AdviceScope.java

...rc/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/ActionInstrumentation.java

...avaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/AdviceScope.java

...rc/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/ActionInstrumentation.java

SylvainJuge

LGTM, some suggestions for a few potential code improvements, mostly to move more code into AdviceScope and inline what is needed there.

...rc/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/ActionInstrumentation.java

...avaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/AdviceScope.java

laurit · 2025-07-15T13:23:22Z

I'd suggest the advice conversion work to be done in a different PR since the main problem with this PR is that the added code attribute isn't useful.

jaydeluca · 2025-07-15T19:38:31Z

I'd suggest the advice conversion work to be done in a different PR since the main problem with this PR is that the added code attribute isn't useful.

I ended up just switching this PR to focus on the advice conversion. If it would be cleaner to close this one and open a fresh one, I'm happy to do that.

SylvainJuge · 2025-07-16T08:30:46Z

I ended up just switching this PR to focus on the advice conversion. If it would be cleaner to close this one and open a fresh one, I'm happy to do that.

Good idea, the scope of this PR is limited to play-2.4, there is also play-2.6 instrumentation that needs to be migrated, but this can be done in a separate PR.

...ent/src/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/Play24Singletons.java

...avaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/AdviceScope.java

SylvainJuge

you can inline and simplify the closeScope method.

...rc/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/ActionInstrumentation.java

laurit · 2025-07-16T13:10:46Z

...rc/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/ActionInstrumentation.java

      }
-      scope.close();

+      actionScope.end(throwable, responseFuture, (Action<?>) thisAction, req);


casting to (Action<?>) is a bit weird. What if you changed @Advice.This Object thisAction to @Advice.This Action<?> thisAction, would that work?

nice, yea that seems to work, thanks

laurit · 2025-07-17T05:23:16Z

there is a failure in play 2.6 tests https://scans.gradle.com/s/36fci3go5n3se

jaydeluca · 2025-07-17T10:27:47Z

...rc/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_6/ActionInstrumentation.java

-      Context parentContext = currentContext();
-      if (!instrumenter().shouldStart(parentContext, null)) {
-        return;
+    public static class AdviceScope {


there is a failure in play 2.6 tests scans.gradle.com/s/36fci3go5n3se

Strangely I haven't been able to reproduce the test failure locally. I am still investigating.

thanks @laurit !

add code attributes to play mvc

b802d15

jaydeluca requested a review from a team as a code owner January 31, 2025 11:19

laurit reviewed Feb 1, 2025

View reviewed changes

SylvainJuge reviewed Jul 11, 2025

View reviewed changes

...avaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/ActionScope.java Outdated Show resolved Hide resolved

jaydeluca added 2 commits July 11, 2025 09:37

Merge branch 'main' of github.com:jaydeluca/opentelemetry-java-instru…

1546f93

…mentation into add-code-attributes-play-mvc

make AdviceScope class consistent, update test and remove deprecated …

0450f64

…semconv

SylvainJuge reviewed Jul 15, 2025

View reviewed changes

code review notes

a6220e8

SylvainJuge approved these changes Jul 15, 2025

View reviewed changes

finish moving logic into advicescope class

f2315e3

remove code attributes

f7d1652

jaydeluca force-pushed the add-code-attributes-play-mvc branch from 288ae9b to d3ab703 Compare July 15, 2025 19:33

no longer need actiondata

ed566b9

jaydeluca force-pushed the add-code-attributes-play-mvc branch from d3ab703 to ed566b9 Compare July 15, 2025 19:35

jaydeluca changed the title ~~Add code attributes to play mvc controller spans~~ Update play mvc to use AdviceScope Jul 15, 2025

laurit reviewed Jul 16, 2025

View reviewed changes

...ent/src/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/Play24Singletons.java Outdated Show resolved Hide resolved

laurit reviewed Jul 16, 2025

View reviewed changes

...avaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/AdviceScope.java Outdated Show resolved Hide resolved

revert instrumenter changes, move advicescope into advice class

760e04d

SylvainJuge reviewed Jul 16, 2025

View reviewed changes

inline scope within advicescope

091d26e

SylvainJuge approved these changes Jul 16, 2025

View reviewed changes

laurit reviewed Jul 16, 2025

View reviewed changes

...rc/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/ActionInstrumentation.java Outdated Show resolved Hide resolved

laurit reviewed Jul 16, 2025

View reviewed changes

jaydeluca added 3 commits July 16, 2025 12:44

fix nesting and cast

a24f019

migrate 2.6 too

4fe2bdd

remove cast

94ebe69

laurit approved these changes Jul 17, 2025

View reviewed changes

move updateSpan into future wrapper

cb48f8f

jaydeluca commented Jul 17, 2025

View reviewed changes

laurit added 2 commits July 18, 2025 19:31

Merge branch 'main' into add-code-attributes-play-mvc

9324fbc

fix indy convertsion

e585f56

trask merged commit 2e4b962 into open-telemetry:main Jul 18, 2025
89 checks passed

SylvainJuge mentioned this pull request Sep 24, 2025

make play-ws and play-mvc indy-ready #14782

Merged

		equalTo(CODE_NAMESPACE, "play.api.mvc.ActionBuilder$$anon$2"),
		equalTo(CODE_FUNCTION, "apply"));

Update play mvc to use AdviceScope #13140

Update play mvc to use AdviceScope #13140

Uh oh!

Conversation

jaydeluca commented Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trask commented Jan 31, 2025

Uh oh!

laurit Feb 1, 2025

Choose a reason for hiding this comment

Uh oh!

SylvainJuge Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SylvainJuge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laurit commented Jul 15, 2025

Uh oh!

jaydeluca commented Jul 15, 2025

Uh oh!

SylvainJuge commented Jul 16, 2025

Uh oh!

Uh oh!

Uh oh!

SylvainJuge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laurit Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

jaydeluca Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

laurit commented Jul 17, 2025

Uh oh!

jaydeluca Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

jaydeluca Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jaydeluca commented Jan 31, 2025 •

edited

Loading